You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Sorted property walk: list a module namespace by name and skip an export that is not set yet - #48066
Three prints of a module namespace throw ReferenceError: Cannot access 'later' before initialization. when an export is not set yet: Bun.inspect(ns, { sorted: true }), a Bun-format snapshot, a failing toEqual() diff. Bun 1.4.4 prints them.
Since bun test: Vitest compatibility #44833, forEachPropertyOrderedImpl (src/jsc/bindings/bindings.cpp:6298) lists enumerable keys only. A namespace reads every export to answer that.
Fix
For a namespace, the walk lists every key, which reads no export (a mocked module aside). Then it drops non-enumerable keys by slot.
Main throws. Bun 1.4.4 prints Module {}. This PR prints Module { hoisted: [Function: hoisted] }, the same as Bun.inspect(ns).
A second way to get such a namespace, with no files: new vm.SourceTextModule(source), then link(), then .namespace. Its exports stay unset until evaluate() runs.
Reach. Fuzzing found this. There is no user report. The commit that brought it in (9e76acc) is after bun-v1.4.4, so no release has it.
Mechanism.JSModuleNamespaceObject::getOwnPropertyNames does a [[GetOwnProperty]] on each export when it is asked for enumerable keys only. The specification requires that for Object.keys(ns), and this PR does not change it. The namespace class of the ahead-of-time run time does the same. Asked for every key, both return the names and read nothing. The unsorted walk already lists the keys that way. One exception: the namespace of a mocked module always lists the object of its mock with the enumerable-only mode.
Jest 30.5.2 with native ES modules: toMatchSnapshot(), toBe() and toEqual() all fail with the ReferenceError.
Vitest 5.1.0: its module objects have no temporal dead zone. The toBe() and toEqual() messages print "later": undefined.
This PR does not print <uninitialized>. It keeps the sorted print equal to the plain print. For a linked node:vm module, the export stays left out until evaluate() runs.
Considered: list every object with every key, as the unsorted walk does, with no test for a namespace. The drop by slot cannot work for every object. The Get slot of a Proxy and of a node:vm global carries no enumerable flag, so their hidden keys would print and their get traps would run for them.
Sorted walk of one plain 3-key object, instructions inside the function (gdb single-step, repeats exactly): release 724 to 731, conditional branches 132 to 134, calls 10 to 10. Debug with ASAN: 1,322 to 1,340, plus 28 in a helper that debug does not inline.
Calls to JSC::AOT::isModuleNamespace added by the walk (gdb hit counts): 0 for 100 plain objects, 0 for 100 compiled objects under bun test --aot, 1 for each --aot namespace printed.
Sorted print of an initialized namespace with 10 exports: 25 export lookups on main, 16 here. The plain print makes 17.
Size: the function grows 96 bytes in release (6,013 to 6,109). Release .text is 110,627,497 bytes in both builds. The debug frame grows 32 bytes (0x740 to 0x760).
Output diff over 144 probe rows (module kinds, matchers, snapshots, process.env, globalThis, Bun, a node:vm global): 24 rows differ. All 24 are sorted prints of a namespace with an unset export. They throw on main and print here.
bun build --compile --aot: 0 of 3 sorted rows throw (main: 3 of 3).
BUN_JSC_validateExceptionChecks=1 on the new tests: 0 reports.
Each clause of the diff has a test that fails without it. Without the isNamespace || term, the hidden-key test in inspect.test.js fails. Without !withNonEnumerable &&, the older-Bun snapshot in snapshot.test.ts fails. Without the --aot arm, the --aot program fails at the compiled run.
One contrived print moves. A namespace that require() gives can take a symbol key from script. With 16 or more keys, and an enumerable symbol whose description equals the name of an export, the sorted print can swap that export and that symbol against main. Example: 15 exports k00 to k14 and Symbol("k05"). Main prints k05 first. This PR prints the symbol first. The order of such a pair was arbitrary before: the sort is not stable and compares a symbol by its description. On a plain object the order changes with the number of keys too, in Bun 1.4.3 as well. No release prints the exports of a namespace in the sorted walk, so no released snapshot holds such a pair.
Still throws after this PR, for a namespace with an unset export.
Part of the regression from 1.4.4, not fixed here:
A snapshot in a test that imports from vitest (src/jsc/bindings/SnapshotFormat.cpp:151). Bun 1.4.4 passed it. That format has its own printer, and Vitest prints "later": undefined there, so it needs its own decision on the output.
Bun.inspect.table(ns) and Bun.inspect.table([ns]): the rows and columns come from another key listing.
expect(ns).toEqual({}): the comparison lists the enumerable keys. Jest throws too.
A Proxy of the namespace in a matcher diff: the Proxy lists its own keys. Bun.inspect prints a Proxy as its target, so that case is fixed.
An unset export named $$typeof, plain and sorted: the printer reads $$typeof before it walks the keys.
A built-in matcher also loses its own message when code of the value throws while the value is printed (a custom inspect hook, a getter). That question is separate. This PR adds no exception clear, so it does not decide it.
no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js, test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts
… that is not set yet
The sorted walk listed the enumerable keys of a value through its
method table. For a module namespace that listing reads every export,
as Object.keys() does, and throws a ReferenceError for an export in its
temporal dead zone. Bun.inspect(ns, { sorted: true }), a Bun-format
snapshot and the diff of a failing toEqual() rethrew that error.
The walk now lists every key of a namespace, which reads no export, and
drops the non-enumerable keys by their slot. The read of each key
already skips one that throws, so the unset export is left out, as in
the plain print. Both namespace classes are covered: the one of the
module loader and the one of the ahead-of-time run time.
The ErrorEvent test in inspect.test.js takes the change of #47123, so
that it no longer depends on its line number.
Reproduced on main b1305dd with a two-file import cycle: a.mjs imports b.mjs, and b.mjs prints the namespace of a.mjs with Bun.inspect(ns, { sorted: true }) while export const later is not set yet. Main throws ReferenceError: Cannot access 'later' before initialization. in all 7 reported rows (sorted Bun.inspect, toMatchSnapshot(), the toEqual() diff). This branch throws in none of them.
CI build 138152: no test that this PR adds or changes fails. The 28 red test files are 27 files under test/aot and test/bundler/compile-aot-size.test.ts. They fail on main too. The commit message of 9e76acc names about 40 test files under test/aot and that size test as known failures.
Tests cover sorted inspection of partially initialized namespaces, linked unevaluated modules, and non-enumerable properties.
Namespace table, diff, and snapshot tests test/js/bun/console/bun-inspect-table.test.ts, test/js/bun/test/expect-extend.test.js, test/js/bun/test/expect.test.js, test/js/bun/test/snapshot-tests/*
Tests cover table output, assertion diffs, and snapshots for partially initialized namespaces, including compatibility with an older snapshot format.
Suggested reviewers: jarred-sumner, dylan-conway, alii
Merge Risk: Minimal · up to 4da88
The change addresses inspection of partially initialized module namespaces, with regression coverage across the affected output formats. No actionable merge-blocking risk is established; merge after normal checks pass.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title names the main change: sorted property listing of a module namespace no longer throws when an export is still uninitialized. It matches the changeset in bindings.cpp (namespace key listing b…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Description check
The description follows the repository template with both required sections, "What does this PR do?" and "How did you verify your code works?". The Problem, Fix, and Background sections state the bug,…
Full details: Title check
Explanation
The title names the main change: sorted property listing of a module namespace no longer throws when an export is still uninitialized. It matches the changeset in bindings.cpp (namespace key listing by name, then drop non-enumerable keys) and the new tests. It is specific and concise, and it is not vague.
Full details: Description check
Explanation
The description follows the repository template with both required sections, "What does this PR do?" and "How did you verify your code works?". The Problem, Fix, and Background sections state the bug, the mechanism, and the change. The Fix section states the approach: list every namespace key without reading exports, then drop non-enumerable keys by slot. The verification section names eight new tests across six files, one --aot program, and a self-review. It also lists measured performance costs, output diffs, and known remaining throws. The text is long, and the Notes block carries much of the detail, but the required information is present and specific. The test run is deferred to CI for platform-specific tests, and the description states this.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
ast-grep (0.51.1)
test/js/bun/test/expect.test.js
ast-grep timed out on this file
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.
The node:vm case that this PR lists under "Still throws" was handed to #47217. I wrote a fix for it there and reviewed it. I will not push it to #47217. The review found that it belongs beside this PR. It also needs a decision from a maintainer first.
What was measured
Bun 1.4.4 (38afee6), the canary of main (7ca0042), and debug builds of the two candidates. Each row is a print of the global of a node:vm context, Bun.inspect(vm.runInContext("this", vm.createContext(sandbox))).
Sandbox
1.4.4
main
Namespace lister
Catch and relist
A namespace with an export that is not set yet, plain and sorted
Module {}
ReferenceError
Module { hoisted: [Function: hoisted] }
Module { hoisted: [Function: hoisted] }
new Proxy(limits, {}), where limits is of a closed node:sqlite database
NodeVMGlobalProxy {}
Error: database is not open
Error: database is not open
{}
A Proxy of { a: 1, b: 2 } whose getOwnPropertyDescriptor trap throws for a
NodeVMGlobalProxy {}
the error of the trap
the error of the trap
{ b: 2 }
A Proxy of { a: 1 } whose ownKeys trap throws one time
NodeVMGlobalProxy {}, 0 trap calls
the error of the trap, 1 call
the error of the trap, 1 call
{ a: 1 }, 2 calls
All four rows come from one statement. Since #44833 the print lists the global with DontEnumPropertiesMode::Exclude, and NodeVMGlobalObject::getOwnPropertyNames hands that mode to the sandbox (src/jsc/bindings/NodeVM.cpp:1657). Bun 1.4.4 did not list the sandbox at all.
Two ways to fix it
1. A lister for a namespace sandbox, with no exception clear. This is what I wrote.
When a namespace is behind the Proxies and contexts of the sandbox, it lists the sandbox with every key. It keeps a key that the namespace has as an enumerable property, which a namespace answers without a read of the binding.
It fixes row 1 only. It is +112/-8 native lines, most of them in NodeVM.cpp.
It has the same namespace test as listsEnumerableKeysByReadingThem in this PR. The two must share one.
The review found two defects in my version, and I confirmed both. The first is for the namespace of a mock.module() whose factory returns a Proxy: a has trap that throws ends the print. The parent commit prints Module { a: 1 } there. The second is for a key that is not an export: the answers of a Proxy sandbox are ignored.
2. Catch and relist at the listing statement. A prototype from the review, below.
It lists with Exclude. If that throws, it clears the error and lists with every key. It keeps a key whose own slot does not throw and is enumerable.
It fixes all four rows, and the bare sorted namespace of this PR, with one helper. It is +39/-3 lines in bindings.cpp and does not touch NodeVM.cpp.
Its price has three parts. It puts an exception clear back on a listing statement, where Propagate JS exceptions instead of clearing them where the caller is fallible #39804 replaced such clears with a return. An ownKeys trap that throws one time no longer ends the print. A print that fails one time asks the getOwnPropertyDescriptor trap of a Proxy sandbox about every builtin of the realm. That is 76 asks for a sandbox with 3 keys, against 2 without the helper.
It was built and run as a debug build only.
The decision
Must rows 2 to 4 print again, as on 1.4.4? Or is their error the intended result since #44833?
If they must print, way 2 fits, and it replaces the namespace arm of this PR.
If the error is intended, way 1 fits, with one predicate shared with this PR and the two defects repaired.
It has 35 tests in test/js/bun/util/inspect.test.js and an --aot program under test/aot/parallel/modules/. 20 of the tests fail without the change. They carry over to both ways, except the two that pin how many times a trap is asked.
Other prints of the global in row 1 still throw with both ways: Bun.inspect.table(g), Bun.inspect.table([g]) and util.inspect(g). Bun 1.4.4 prints all three.
The follow-up for Jest's and Vitest's snapshot formats is #48264.
It prints an export that is not initialized as undefined in those two formats. Vitest and Jest with Babel write the same.
A merge of this branch (4da88e0) into it has no conflict. On a release build of the merge, the new tests of both PRs pass.
One route passes only on the merge and has no test yet: an inline snapshot under bun:test that holds what Jest writes ("later": undefined). The PR that lands second can add that case to test/js/bun/test/snapshot-tests/formats/jest/inline.fixture.js.
For the decision that the comment above asks for, the listing in bun test: print an unset namespace export as undefined in Jest and Vitest snapshots #48264 is a third variant. It lists again only for a module namespace. It clears the error only when the export that the listing threw at is a variable of a source text module that is still empty. It throws every other error again, and it asks no mock and no Proxy a second time.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Problem
ReferenceError: Cannot access 'later' before initialization.when an export is not set yet:Bun.inspect(ns, { sorted: true }), a Bun-format snapshot, a failingtoEqual()diff. Bun 1.4.4 prints them.forEachPropertyOrderedImpl(src/jsc/bindings/bindings.cpp:6298) lists enumerable keys only. A namespace reads every export to answer that.Fix
test/js/bun/util/inspect.test.jsand five other files, plus one--aotprogram. All but one fail on main.Background
import * as nsgives. Aconst,letorclassexport throws when read before its declaration runs, as in an import cycle.Bun.inspectwithsorted, Bun-format snapshots and matcher diffs.bun test --aotnamespace class andexpect.objectContaining(ns).Downsides
vitest, and anode:vmcontext with such a sandbox.Notes
Repro, run as
bun a.mjs:Main throws. Bun 1.4.4 prints
Module {}. This PR printsModule { hoisted: [Function: hoisted] }, the same asBun.inspect(ns).A second way to get such a namespace, with no files:
new vm.SourceTextModule(source), thenlink(), then.namespace. Its exports stay unset untilevaluate()runs.Reach. Fuzzing found this. There is no user report. The commit that brought it in (9e76acc) is after
bun-v1.4.4, so no release has it.Mechanism.
JSModuleNamespaceObject::getOwnPropertyNamesdoes a[[GetOwnProperty]]on each export when it is asked for enumerable keys only. The specification requires that forObject.keys(ns), and this PR does not change it. The namespace class of the ahead-of-time run time does the same. Asked for every key, both return the names and read nothing. The unsorted walk already lists the keys that way. One exception: the namespace of a mocked module always lists the object of its mock with the enumerable-only mode.What other tools do (run here):
util.inspect(ns, { sorted: true })printslater: <uninitialized>.Object.keys(ns)throws.toMatchSnapshot(),toBe()andtoEqual()all fail with the ReferenceError.toBe()andtoEqual()messages print"later": undefined.This PR does not print
<uninitialized>. It keeps the sorted print equal to the plain print. For a linkednode:vmmodule, the export stays left out untilevaluate()runs.Considered: list every object with every key, as the unsorted walk does, with no test for a namespace. The drop by slot cannot work for every object. The
Getslot of a Proxy and of anode:vmglobal carries no enumerable flag, so their hidden keys would print and theirgettraps would run for them.Measurements, main b1305dd against this PR:
JSC::AOT::isModuleNamespaceadded by the walk (gdb hit counts): 0 for 100 plain objects, 0 for 100 compiled objects underbun test --aot, 1 for each--aotnamespace printed..textis 110,627,497 bytes in both builds. The debug frame grows 32 bytes (0x740to0x760).process.env,globalThis,Bun, anode:vmglobal): 24 rows differ. All 24 are sorted prints of a namespace with an unset export. They throw on main and print here.bun build --compile --aot: 0 of 3 sorted rows throw (main: 3 of 3).BUN_JSC_validateExceptionChecks=1on the new tests: 0 reports.isNamespace ||term, the hidden-key test ininspect.test.jsfails. Without!withNonEnumerable &&, the older-Bun snapshot insnapshot.test.tsfails. Without the--aotarm, the--aotprogram fails at the compiled run.One contrived print moves. A namespace that
require()gives can take a symbol key from script. With 16 or more keys, and an enumerable symbol whose description equals the name of an export, the sorted print can swap that export and that symbol against main. Example: 15 exportsk00tok14andSymbol("k05"). Main printsk05first. This PR prints the symbol first. The order of such a pair was arbitrary before: the sort is not stable and compares a symbol by its description. On a plain object the order changes with the number of keys too, in Bun 1.4.3 as well. No release prints the exports of a namespace in the sorted walk, so no released snapshot holds such a pair.Still throws after this PR, for a namespace with an unset export.
Part of the regression from 1.4.4, not fixed here:
vitest(src/jsc/bindings/SnapshotFormat.cpp:151). Bun 1.4.4 passed it. That format has its own printer, and Vitest prints"later": undefinedthere, so it needs its own decision on the output.node:vmcontext whose sandbox is the namespace or a Proxy of it, plain and sorted. Bun 1.4.4 printedModule {}. Since bun test: Vitest compatibility #44833 the global forwards the listing mode to its sandbox (src/jsc/bindings/NodeVM.cpp:1657). inspect: print a node:vm context global from its own property slots, sorted or not #47217 rewrites that listing and is the place for it. Its current head still throws.New with #44833, never printed:
Reflect.ownKeys()and the plain print too. Bun 1.4.4 fails at the import of the mock. bun test: describe a mocked module's export without calling its getter #47815 works in that area.Older than the regression, Bun 1.4.4 throws too:
Bun.inspect.table(ns)andBun.inspect.table([ns]): the rows and columns come from another key listing.expect(ns).toEqual({}): the comparison lists the enumerable keys. Jest throws too.Proxyof the namespace in a matcher diff: the Proxy lists its own keys.Bun.inspectprints a Proxy as its target, so that case is fixed.$$typeof, plain and sorted: the printer reads$$typeofbefore it walks the keys.A built-in matcher also loses its own message when code of the value throws while the value is printed (a custom inspect hook, a getter). That question is separate. This PR adds no exception clear, so it does not decide it.
Tests.
ErrorEventtest ininspect.test.jstakes the same change as test: make the ErrorEvent inspect snapshot independent of its line number #47123. The test moves past line 999 here, and its old snapshot depended on the width of the line numbers.snapshot.test.ts, wherebun test -ucannot rewrite it.--aotcase is a program undertest/aot/parallel/modules/.run-parallel.tsfails it on a release build of main and passes it here.console.logformatter #46729 (head 070a72c), which replaces the Rust formatter of Bun-format snapshots, all eight tests pass with no further change.Nearby open PRs that edit the same function or its callers: #47217, #46156, #47799, #37331, #38937, #47861, #46729, #47605, #47815, #47788, #47123. None removes this throw.
no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/util/inspect.test.js, test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts